Why would this already be patched? Shouldn't we just no-op if it is already wrapped?

with web there is always some probability that something can be destroyed and recreated again. This way it will always work correctly

Since this is patching a global, would it not patch if multiple components on the same page are instrumented with separate tracers/configs?

copy paste here my reply from below
We are using shimmer which doesn't support multiple wrap of the same function so if you even register multiple tracers with the same plugins, and try to unpatch just one, it will unpatch the global successfully anyway. So if the decision to use shimmer was done my assumption is that we are able to instrument using the plugins once only. Otherwise the code will be broken anyway.

it is always safer to use original.apply(this, arguments) because we can't account for users passing incorrect values or the incorrect number of values. It also removes the need for this check.

It is the same here.
You don't have arguments word in typescript , so you would still declare array of arguments and define type for it, which would be quite weird anyway.

Besides that check also the whole code to see more context. The async will always be set to true and there are reasons for that check here. It is also worth to mention that it was ported from open census.

You do have the arguments keyword in typescript.

It is possible to call open with async: false and user not-null. In this case you are changing the arguments the user has called the function with.

We cannot assume that users will use the API properly as documented and it is much easier and safer to directly pass their arguments. If the user incorrectly calls open with async: true I am suggesting something like this (tests pass with this version):

  protected _patchOpen() {
    return (original: Function): OpenFunction => {
      const plugin = this;
      return function patchOpen(
        this: XMLHttpRequest,
        method: string,
        url: string,
        async?: boolean,
        user?: string | null,
        pass?: string | null
      ): ReturnType<OpenFunction> {
        plugin._createSpan(this as XMLHttpRequestWrapped, url, method);
        return original.apply(this, arguments);
        /*
        If you would rather use `call` this can be done easily
        return original.call(this, ...arguments);
        */
      };
    };
  }

Regarding the performance difference between apply and call, it may be the case that call is faster than apply, but in this case the if statement would provide more overhead than the difference.

still cannot use apply anyway, as I mentioned previously please read the spec, and why it needs to force the async

If true, notification of a completed transaction is provided using event listeners.

What about that means it must be true? If it is false (synchronous), then you just finish the span after calling the original function. If user sets async false in a context that it is not allowed (some browsers only allow sync requests in workers), then

The way your code is currently, it changes the behavior of user code behind the scenes. If I am a user and I make a synchronous xhr request, I expect it to be made synchronously not transparently rewritten to be async behind the scenes by my telemetry provider.

https://github.com/census-instrumentation/opencensus-web/blob/68aa988732e03f2de49ebc0213c3b2525aa5b68b/packages/opencensus-web-instrumentation-zone-peer-dep/src/monkey-patching.ts#L31
and
https://github.com/census-instrumentation/opencensus-web/blob/68aa988732e03f2de49ebc0213c3b2525aa5b68b/packages/opencensus-web-instrumentation-zone-peer-dep/src/monkey-patching.ts#L54
and
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/Synchronous_and_Asynchronous_Requests

Can you guys @draffensperger @mayurkale22 have your input here too, thx :).

and one more Synchronous XHR is now in deprecation state.

I understand that it's deprecated, but that doesn't mean users can't or won't use it and the telemetry library shouldn't silently change the behavior of the program.

should we tallk about this in the SIG tomorrow?

1. call is faster
2. we already know there is just one para, we know it's type so we can use it and typescript will also validate the type of this is correcty
3. You don't have arguments word in typescript , so you would still declare array of aguments and define type for it, which would be quite weird anyway

and the same here, thx
@draffensperger @mayurkale22 ^^

That would assume that the user has the same version of the plugin and of core installed which may not always be the case. @obecny and I had talked about adding the autogenerated VERSION file to all lerna packages, but nothing has been done yet.

You do have the arguments keyword in typescript.

It is possible to call open with async: false and user not-null. In this case you are changing the arguments the user has called the function with.

We cannot assume that users will use the API properly as documented and it is much easier and safer to directly pass their arguments. If the user incorrectly calls open with async: true I am suggesting something like this (tests pass with this version):

  protected _patchOpen() {
    return (original: Function): OpenFunction => {
      const plugin = this;
      return function patchOpen(
        this: XMLHttpRequest,
        method: string,
        url: string,
        async?: boolean,
        user?: string | null,
        pass?: string | null
      ): ReturnType<OpenFunction> {
        plugin._createSpan(this as XMLHttpRequestWrapped, url, method);
        return original.apply(this, arguments);
        /*
        If you would rather use `call` this can be done easily
        return original.call(this, ...arguments);
        */
      };
    };
  }

Regarding the performance difference between apply and call, it may be the case that call is faster than apply, but in this case the if statement would provide more overhead than the difference.

I think this will be a pretty routine case that the user sends an XHR to a different origin that they don't want to pass along trace headers for (that is, at least until the browser spec can give an exception for CORS for the W3C trace headers)

Given that, would it make sense to remove this warning log and corresponding if block?

I might be wrong but my understanding is that we might break something with the request if we force to add headers. The headers are only added when origin is the same. If not then I check if user allows to add the headers based on config. If not then the headers are dropped and warning is being show. I cannot remove the if block (to prevent adding headers) but only warning if that is something you want me to do ?

Creating a new thread on this line as the old one is deep in the history and on a line that has been outdated for a long time.

@draffensperger @mayurkale22

Yeah, I think I missed this in my review, but I definitely agree that we should preserve the original behavior of the XHR function including the async parameter.

I believe if there is no current span, this span will just be a root span, which is the correct behavior. I think my comment about having a nice root span for XHRs is more like a future feature and not so much an issue with this code.